[Choregraphy] Upgrade EventGrid sdk from t1 to t2#75
[Choregraphy] Upgrade EventGrid sdk from t1 to t2#75hallihan merged 5 commits intomspnp:masterfrom zedy-wj:Choregraphy-EventGrid
Conversation
...choreography/Fabrikam.Choreography.ChoreographyService/Controllers/ChoreographyController.cs
Outdated
Show resolved
Hide resolved
|
@jsquire - can you please review? |
...choreography/Fabrikam.Choreography.ChoreographyService/Controllers/ChoreographyController.cs
Outdated
Show resolved
Hide resolved
...raphy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
Outdated
Show resolved
Hide resolved
...oreography/Frabrikam.Choreography.ChoreographyService.Tests/ChoreographyControllerFixture.cs
Outdated
Show resolved
Hide resolved
...oreography/Frabrikam.Choreography.ChoreographyService.Tests/ChoreographyControllerFixture.cs
Outdated
Show resolved
Hide resolved
|
Hi,@jsquire - Thanks for your review! Updated according to your comments. |
...choreography/Fabrikam.Choreography.ChoreographyService/Controllers/ChoreographyController.cs
Outdated
Show resolved
Hide resolved
...raphy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
Outdated
Show resolved
Hide resolved
...raphy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
Outdated
Show resolved
Hide resolved
...raphy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
Outdated
Show resolved
Hide resolved
...raphy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
Outdated
Show resolved
Hide resolved
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.Azure.EventGrid; | ||
| using Microsoft.Azure.EventGrid.Models; | ||
| using Azure.Messaging.EventGrid; |
There was a problem hiding this comment.
Truthfully, though we didn't change the behavior with the upgrade, we should really look at this controller in more depth. The logic is convoluted and it's demonstrating a really bad pattern of HTTP responses where any downstream failures are returned to the caller as an HTTP 400 (Bad Request) indicating "you did something wrong when sending your payload and you need to fix it for this to work."
There was a problem hiding this comment.
The coding conventions also often cut against best practices for C#. For example:
try
{
...
}
catch (Exception ex) when (ex is SomeExceptionType)
{
...
}Should just be:
try
{
...
}
catch (SomeExceptionType ex)
{
...
}There was a problem hiding this comment.
We have known the existence of this problem, we will improve the logic after completing all migrations. Our primary task is upgrading sdk from t1 to t2.
There was a problem hiding this comment.
I realize this request expands the scope, but let's get a better design here as well. Do you want to take a stab at it with Jesse's recommendation above or do you need further guidance?
There was a problem hiding this comment.
We will take a stab at it, but it maybe takes a few time.
1.Upgrade EventGrid sdk from t1 to t2
2.Update files:
@jongio for notification.